Wp 5257/create durable nonce solana recovery by mohammadalfaiyazbitgo · Pull Request #70 · BitGo/advanced-wallets

mohammadalfaiyazbitgo · 2025-07-15T18:35:29Z

No description provided.

pranavjain97 · 2025-07-16T19:45:35Z

+    let unsignedSweepPrebuildTx: Awaited<ReturnType<typeof recoverEddsaWallets>>;
+    if (sdkCoin.getFamily() === CoinFamily.SOL) {
+      const solanaParams = params.coinSpecificParams as SolanaRecoveryOptions;
+      console.log(params);


Please use logger

pranavjain97 · 2025-07-16T19:45:41Z

+    let unsignedSweepPrebuildTx: Awaited<ReturnType<typeof recoverEddsaWallets>>;
+    if (sdkCoin.getFamily() === CoinFamily.SOL) {
+      const solanaParams = params.coinSpecificParams as SolanaRecoveryOptions;
+      console.log(params);


pranavjain97 · 2025-07-16T19:47:35Z

          walletContractAddress: '',
          unsignedSweepPrebuildTx: undefined,
-          coinSpecificParams: undefined,
+          coinSpecificParams: coinSpecificParams?.solanaRecoveryOptions,


kinda oddd to have solanaRecoveryOptions for handleEddsaRecovery.

yeah but for now it's the only coin with additional options, so kept it like this.

pranavjain97 · 2025-07-16T19:48:32Z

+  // Solana specific recovery parameters
+  solanaRecoveryOptions: t.partial({


are these solana specific, or eddsa specific? or is a subset of these for solana only?

solana specific.

woudnt this be confusing then? what would be the expected params for non-sol eddsa recoveries?

export interface MPCRecoveryOptions { userKey?: string; // Box A backupKey?: string; // Box B bitgoKey: string; // Box C - this is bitgo's xpub and will be used to derive their root address recoveryDestination: string; walletPassphrase?: string; seed?: string; index?: number; // the starting receive address index to scan from for WRW recovery startingScanIndex?: number; // the number of addresses to scan from the starting index scan?: number; // token contract address of the token to recover tokenContractAddress?: string; apiKey?: string; }``` that's the recovery options for other eddsa coins, so those options are already encapsulated in our base parameters. Only sol has additional options.

pranavjain97 · 2025-07-16T19:49:10Z

+    };
+    let unsignedSweepPrebuildTx: Awaited<ReturnType<typeof recoverEddsaWallets>>;
+    if (sdkCoin.getFamily() === CoinFamily.SOL) {
+      const solanaParams = params.coinSpecificParams as SolanaRecoveryOptions;


why are we not using params.solanaSpecificParams that u added here?

poor structuring of the parameters

it's passed in as

coinSpecificParams: coinSpecificParams?.solanaRecoveryOptions,

Ticket: WP-5300 # Conflicts: # src/api/master/routers/masterApiSpec.ts

Copilot

Pull Request Overview

This PR implements durable nonce support for Solana recovery and restructures the coin-specific parameters for recovery operations. The changes include creating reusable transaction utilities, standardizing error handling, and expanding parameter validation for different coin types.

Key changes:

Adds transaction utility functions for handling various transaction formats
Implements structured coin-specific recovery parameters for UTXO, EVM, and Solana coins
Introduces comprehensive error handling classes with appropriate HTTP status codes
Adds support for Solana durable nonce in recovery operations

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/shared/transactionUtils.ts	New utility module for transaction type checking and data extraction
src/shared/errors.ts	New custom error classes with HTTP status code mapping
src/shared/responseHandler.ts	Updates error handling to support new custom error types
src/api/master/routers/masterApiSpec.ts	Restructures API spec with coin-specific recovery parameters
src/api/master/handlers/recoveryWallet.ts	Implements parameter validation and Solana durable nonce support
src/api/master/clients/enclavedExpressClient.ts	Refactors to use new transaction utility functions
src/tests/api/master/recoveryWallet.test.ts	Comprehensive test coverage for parameter validation
masterBitgoExpress.json	Updates JSON schema to reflect new parameter structure

Copilot · 2025-07-17T19:06:04Z

-    });
+    };
+    let unsignedSweepPrebuildTx: Awaited<ReturnType<typeof recoverEddsaWallets>>;
+    if (sdkCoin.getFamily() === CoinFamily.SOL) {


Type assertion without null checking could cause runtime errors. The coinSpecificParams could be undefined, and accessing properties on solanaParams would fail if it's undefined.

Suggested change

if (sdkCoin.getFamily() === CoinFamily.SOL) {

if (sdkCoin.getFamily() === CoinFamily.SOL) {

if (!params.coinSpecificParams) {

throw new ValidationError('coinSpecificParams is required for Solana recovery');

}

Copilot · 2025-07-17T19:06:04Z

+    txRequest.signableHex = firstTransaction.unsignedTx?.serializedTx || '';
+    txRequest.derivationPath = firstTransaction.unsignedTx?.derivationPath || '';
+  } else {
+    throw new Error(`Unrecognized transaction ${JSON.stringify(tx)}`);


JSON.stringify on untrusted input could potentially expose sensitive data or cause performance issues with large objects. Consider logging a generic error message instead.

Suggested change

throw new Error(`Unrecognized transaction ${JSON.stringify(tx)}`);

throw new Error('Unrecognized transaction format.');

Error would be thrown back to client, not a concern.

Copilot · 2025-07-17T19:06:05Z

+ * Type guard to check if the object is an MPCTxs format
+ * MPCTxs format has a txRequests array containing transaction information
+ */
+export function isMPCTxs(tx: any): tx is MPCTxs {


[nitpick] Using 'any' type reduces type safety. Consider using 'unknown' instead to force proper type checking at call sites.

Suggested change

export function isMPCTxs(tx: any): tx is MPCTxs {

export function isMPCTxs(tx: unknown): tx is MPCTxs {

Copilot · 2025-07-17T19:06:05Z

+}
+
+export function isMPCUnsignedTx(tx: any): tx is MPCUnsignedTx {
+  return 'unsignedTx' in tx && isMPCTx(tx);


The type guard logic is incorrect. isMPCUnsignedTx should check for 'unsignedTx' property but then calls isMPCTx which checks for 'signableHex'. An MPCUnsignedTx likely has a different structure than MPCTx.

Suggested change

return 'unsignedTx' in tx && isMPCTx(tx);

return tx && 'unsignedTx' in tx && typeof tx.unsignedTx === 'object';

what? https://github.com/BitGo/BitGoJS/blob/1081629773dc93e2b8f719711a670a8a88c316d3/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts#L525 you're tripping.

Copilot · 2025-07-17T19:06:05Z

+      const solanaRecoveryOptions: SolRecoveryOptions = { ...options };
+      solanaRecoveryOptions.recoveryDestinationAtaAddress =
+        solanaParams.recoveryDestinationAtaAddress;
+      solanaRecoveryOptions.closeAtaAddress = solanaParams.closeAtaAddress;
+      solanaRecoveryOptions.tokenContractAddress = solanaParams.tokenContractAddress;
+      solanaRecoveryOptions.programId = solanaParams.programId;
+      if (solanaParams.durableNonce) {
+        solanaRecoveryOptions.durableNonce = {
+          publicKey: solanaParams.durableNonce.publicKey,
+          secretKey: solanaParams.durableNonce.secretKey,
+        };
+      }


[nitpick] The spread operator creates a shallow copy, but then individual properties are assigned. Consider directly constructing the object to be more explicit about the intended structure.

Suggested change

const solanaRecoveryOptions: SolRecoveryOptions = { ...options };

solanaRecoveryOptions.recoveryDestinationAtaAddress =

solanaParams.recoveryDestinationAtaAddress;

solanaRecoveryOptions.closeAtaAddress = solanaParams.closeAtaAddress;

solanaRecoveryOptions.tokenContractAddress = solanaParams.tokenContractAddress;

solanaRecoveryOptions.programId = solanaParams.programId;

if (solanaParams.durableNonce) {

solanaRecoveryOptions.durableNonce = {

publicKey: solanaParams.durableNonce.publicKey,

secretKey: solanaParams.durableNonce.secretKey,

};

}

const solanaRecoveryOptions: SolRecoveryOptions = {

bitgoKey: options.bitgoKey,

recoveryDestination: options.recoveryDestination,

apiKey: options.apiKey,

recoveryDestinationAtaAddress: solanaParams.recoveryDestinationAtaAddress,

closeAtaAddress: solanaParams.closeAtaAddress,

tokenContractAddress: solanaParams.tokenContractAddress,

programId: solanaParams.programId,

durableNonce: solanaParams.durableNonce

? {

publicKey: solanaParams.durableNonce.publicKey,

secretKey: solanaParams.durableNonce.secretKey,

}

: undefined,

};

mohammadalfaiyazbitgo requested review from Copilot and pranavjain97 July 15, 2025 18:35

This comment was marked as outdated.

Sign in to view

mohammadalfaiyazbitgo force-pushed the WP-5257/create-durable-nonce-solana-recovery branch from 7ee0805 to 91f0ce7 Compare July 15, 2025 20:26

mohammadalfaiyazbitgo requested a review from Copilot July 15, 2025 20:27

mohammadalfaiyazbitgo marked this pull request as ready for review July 15, 2025 20:27

This comment was marked as outdated.

Sign in to view

pranavjain97 requested changes Jul 16, 2025

View reviewed changes

mohammadalfaiyazbitgo added 3 commits July 17, 2025 13:42

feat: add durable nonce for sol recovery

b586b84

feat: Error Handling for MBE with Common Error Classes

e71ff94

Ticket: WP-5300 # Conflicts: # src/api/master/routers/masterApiSpec.ts

chore: add type guarding for mpc recovery

50a2edb

mohammadalfaiyazbitgo force-pushed the WP-5257/create-durable-nonce-solana-recovery branch from 91f0ce7 to 50a2edb Compare July 17, 2025 19:04

mohammadalfaiyazbitgo requested review from Copilot and pranavjain97 July 17, 2025 19:04

Copilot AI reviewed Jul 17, 2025

View reviewed changes

pranavjain97 approved these changes Jul 17, 2025

View reviewed changes

mohammadalfaiyazbitgo merged commit fd128b6 into master Jul 17, 2025
3 checks passed

mohammadalfaiyazbitgo deleted the WP-5257/create-durable-nonce-solana-recovery branch July 17, 2025 20:38

		// Solana specific recovery parameters
		solanaRecoveryOptions: t.partial({

	throw new Error(`Unrecognized transaction ${JSON.stringify(tx)}`);
	throw new Error('Unrecognized transaction format.');

	export function isMPCTxs(tx: any): tx is MPCTxs {
	export function isMPCTxs(tx: unknown): tx is MPCTxs {

	return 'unsignedTx' in tx && isMPCTx(tx);
	return tx && 'unsignedTx' in tx && typeof tx.unsignedTx === 'object';

Conversation

mohammadalfaiyazbitgo commented Jul 15, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants